Validate Guest Address Ranges for Overlapping Regions in map_region#1464
Validate Guest Address Ranges for Overlapping Regions in map_region#1464Richard-Durkee wants to merge 1 commit into
Conversation
644165f to
bb16213
Compare
ludfjig
left a comment
There was a problem hiding this comment.
thanks for your contribution!
I think we can drop the validation in MultiUseSandbox::map_region entirely and rely on the check inside HyperlightVm::map_region, then reorder so the call happens before the snapshot reset:
unsafe { self.vm.map_region(rgn) }.map_err(HyperlightVmError::MapRegion)?;
self.snapshot = None;
self.mem_mgr.mapped_rgns += 1;The same pattern can probably applies to map_file_cow too I think. What do you think about this?
3075ff3 to
f2c4072
Compare
Thanks for the feedback! This makes sense to me. I just pushed again and included these suggestions. Please let me know if there are any other changes you'd like. |
Looks great! Just one minor thing I just realized the page-table "tail" of compacted snapshots are not mapped into the vm, so the could be less than #[cfg(not(unshared_snapshot_mem))]
let snap_end = snap_start + snapshot.guest_mapped_size();
#[cfg(unshared_snapshot_mem)]
let snap_end = snap_start + snapshot.mem_size();should do it. Thanks! |
cfd33a6 to
fa24c67
Compare
|
Ah, okay -- I just learned about compacted snapshots. Pushed the change. Thanks again for the help. |
fa24c67 to
5f016ed
Compare
danbugs
left a comment
There was a problem hiding this comment.
Mostly nits, nothing blocking. LGTM.
5f016ed to
5e09e95
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR centralizes guest-memory overlap detection inside HyperlightVm::map_region and adjusts sandbox snapshot invalidation to occur only after successful mappings, adding tests to validate overlap behavior.
Changes:
- Add overlap detection (dynamic regions + snapshot + scratch) to
HyperlightVm::map_regionvia a newMapRegionError::Overlappingvariant. - Move
MultiUseSandboxsnapshot reset to occur aftermap_regionsucceeds. - Add unit tests covering overlapping, partially overlapping, adjacent, and reserved-region overlap scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/hyperlight_host/src/sandbox/initialized_multi_use.rs |
Reorders snapshot reset after successful mapping; removes redundant overlap scanning; adds overlap-focused tests. |
src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs |
Implements centralized overlap checks and surfaces them through a dedicated Overlapping mapping error. |
Comments suppressed due to low confidence (1)
src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs:1
- Adding a new variant to a
pub enumis a semver-breaking change for downstream crates that exhaustivelymatchonMapRegionError. If this type is part of the public API surface, consider marking the enum#[non_exhaustive](and updating callers accordingly), or otherwise gating this behind a non-public error type / major version bump.
/*
| let snap_start = crate::mem::layout::SandboxMemoryLayout::BASE_ADDRESS; | ||
| #[cfg(not(unshared_snapshot_mem))] | ||
| let snap_end = snap_start + snapshot.guest_mapped_size(); | ||
| #[cfg(unshared_snapshot_mem)] | ||
| let snap_end = snap_start + snapshot.mem_size(); | ||
| if new_start < snap_end && new_end > snap_start { | ||
| return Err(MapRegionError::Overlapping { | ||
| new_start, | ||
| new_end, | ||
| existing_start: snap_start, | ||
| existing_end: snap_end, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Check against the scratch region | ||
| if let Some(ref scratch) = self.scratch_memory { | ||
| let scratch_start = | ||
| hyperlight_common::layout::scratch_base_gpa(scratch.mem_size()) as usize; | ||
| let scratch_end = scratch_start + scratch.mem_size(); | ||
| if new_start < scratch_end && new_end > scratch_start { | ||
| return Err(MapRegionError::Overlapping { | ||
| new_start, | ||
| new_end, | ||
| existing_start: scratch_start, | ||
| existing_end: scratch_end, | ||
| }); | ||
| } | ||
| } |
| let err = unsafe { sbox.map_region(®ion2) }.unwrap_err(); | ||
| assert!( | ||
| format!("{err:?}").contains("Overlapping"), | ||
| "Expected Overlapping error, got: {err:?}" | ||
| ); |
| #[test] | ||
| fn map_region_rejects_overlapping_regions() { | ||
| let mut sbox: MultiUseSandbox = { | ||
| let path = simple_guest_as_string().unwrap(); | ||
| let u_sbox = UninitializedSandbox::new(GuestBinary::FilePath(path), None).unwrap(); | ||
| u_sbox.evolve().unwrap() | ||
| }; |
Closes hyperlight-dev#1289 Signed-off-by: Richard Durkee <Richard-Durkee@users.noreply.github.com>
5e09e95 to
7b5b668
Compare
Add overlap validation to
HyperlightVm::map_regionto enforce the safety contract documented onVirtualMachine::map_memory, which requires non-overlapping regions.Checks the new region against existing dynamically mapped regions (
mmap_regions), the snapshot region (starting atBASE_ADDRESS), and the scratch region (at the top of the guest physical address space).Adds
Overlappingvariant toMapRegionErrorwith a descriptive message showing both the new and conflicting ranges.Also adds early validation at the
MultiUseSandbox::map_regionlevel to reject invalid input before side effects (snapshot reset) occur.Tested on KVM (
c8i.2xlargewith nested virtualization enabled).Closes #1289